Fix reasoning_content error in tool calls for kimi-k2.5 and other thi…#876
Fix reasoning_content error in tool calls for kimi-k2.5 and other thi…#876jdhxyy wants to merge 1 commit intosipeed:mainfrom
Conversation
nikolasdehor
left a comment
There was a problem hiding this comment.
Good targeted fix for #588. The field naming (ReasoningContent) is consistent with the existing Message struct. Two observations: (1) Incomplete fix? This PR preserves reasoning_content in the outbound serialization (stripSystemParts), but I don't see where response.ReasoningContent is copied into the assistant Message during tool loop execution. Without that, the ReasoningContent field will be empty for assistant messages created in toolloop.go. PR #889 addresses this in toolloop.go:109 — you may need to add that piece here too, or coordinate with that PR. (2) Unrelated .gitignore change: The __debug_bin patterns are fine but would be cleaner as a separate commit. (3) Test: Consider adding a test for stripSystemParts that verifies ReasoningContent is preserved. The core approach is correct — please verify the tool loop path is also covered.
🔍 Deep Code ReviewThanks for the fix! The approach is correct, but I have some suggestions. 🔬 Problem AnalysisRoot cause verified:
This caused:
✅ Fix AssessmentThe fix is correct - adding
|
| Type | Count |
|---|---|
| 🚨 Critical Issues | 0 |
| 1 (missing request serialization test) | |
| 💡 Suggestions | 2 |
Verdict: The fix is correct and can be merged after adding the test. Please add the test case above to ensure reasoning_content is properly preserved in multi-turn conversations.
…nking models
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Fixes #588
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist